Skip to content

Conversation

aaradhak
Copy link
Member

backporting #4181

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request backports a failing test for FIPS and LUKS. The new test verifies that cryptsetup fails as expected when using a FIPS-incompatible algorithm. The code is well-structured, but I have a few suggestions to improve clarity and conciseness.

Comment on lines +98 to +100
if err := ignitionFailure(c); err != nil {
c.Fatal(err.Error())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It's more idiomatic to pass the error object directly to c.Fatal() instead of err.Error(). The testing harness will handle formatting it as a string, and this preserves the error type information.

Suggested change
if err := ignitionFailure(c); err != nil {
c.Fatal(err.Error())
}
if err := ignitionFailure(c); err != nil {
c.Fatal(err)
}

Comment on lines +106 to +119
func fileContainsPattern(path string, searchPattern string) (bool, error) {
file, err := os.ReadFile(path)
if err != nil {
return false, err
}
// File has content, but the pattern is not present
match := regexp.MustCompile(searchPattern).Match(file)
if match {
// Pattern found
return true, nil
}
// Pattern not found
return false, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This function can be simplified. The comments are not very helpful, and using regexp.Match would be more concise and idiomatic Go. regexp.Match directly returns (bool, error), which matches the function signature, and handles potential regexp compilation errors gracefully unlike regexp.MustCompile which would panic.

func fileContainsPattern(path string, searchPattern string) (bool, error) {
	file, err := os.ReadFile(path)
	if err != nil {
		return false, err
	}
	return regexp.Match(searchPattern, file)
}

Comment on lines +174 to +184
failConfig, err := failConfig.Render(conf.FailWarnings)
if err != nil {
return errors.Wrapf(err, "creating invalid FIPS config")
}

// Create a temporary log file
consoleFile := c.H.TempFile("console-")

// Instruct builder to use it
builder.ConsoleFile = consoleFile.Name()
builder.SetConfig(failConfig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The variable failConfig is shadowed inside this function. The global failConfig is a *conf.UserData, while the local failConfig returned by failConfig.Render() is a *conf.Conf. This can be confusing. Renaming the local variable would improve clarity.

renderedConfig, err := failConfig.Render(conf.FailWarnings)
	if err != nil {
		return errors.Wrapf(err, "creating invalid FIPS config")
	}

	// Create a temporary log file
	consoleFile := c.H.TempFile("console-")

	// Instruct builder to use it
	builder.ConsoleFile = consoleFile.Name()
	builder.SetConfig(renderedConfig)

@aaradhak aaradhak changed the title Backport failing test for FIPS & LUKS [rhcos-4.19] - Backport failing test for FIPS & LUKS Aug 13, 2025
Copy link

openshift-ci bot commented Aug 13, 2025

@aaradhak: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/rhcos a951601 link true /test rhcos

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@aaradhak aaradhak closed this Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant